Skip to content

fix: decode GitHub source paths before installing skills - #1069

Open
anticipate218 wants to merge 1 commit into
evalstate:mainfrom
anticipate218:fix/decode-github-skill-paths
Open

anticipate218 wants to merge 1 commit into
evalstate:mainfrom
anticipate218:fix/decode-github-skill-paths

Conversation

@anticipate218

Copy link
Copy Markdown

GitHub skill URLs can contain escaped repository paths, but parsing currently passes that escaped text to the filesystem. For example, a marketplace entry pointing at .../tree/main/skills/hello%20world tries to install from the literal skills/hello%20world directory and fails even when skills/hello world/SKILL.md exists.

Decode repository paths once when parsing GitHub web/raw URLs, and encode them when constructing raw-content URLs. Explicit repo_path values retain their existing filesystem semantics. Reference parsing and the existing slash-branch heuristics are unchanged. Paths containing invalid UTF-8, leading/trailing whitespace or backslashes are rejected at the URL boundary because the existing path normalizer cannot represent them without changing the target.

The regression creates real temporary Git repositories and exercises marketplace parsing followed by skill installation, replacing only the remote-clone boundary. It covers spaces, Unicode, literal percent escapes and #, URL round trips, and rejection of paths that could otherwise select a different directory.

Validation on Windows / CPython 3.13.5 against c551658338782a5fd7feb161902504c8cf429e6d:

  • Original implementation: 22 failures and 3 passing controls in the final 25 cases. Fixed: all 25 pass.
  • Complete marketplace, skills and plugins unit directories: 436 passed, 1 failed. The same Windows slash-format assertion in test_schema_v2_manifest_loads_post_user_turn_hook also fails on the unchanged base.
  • Independent URL, containment and real-directory checks: 171 passed.
  • Required lint script with its inline-pinned Ruff 0.16.8 passes.
  • Required format script reports one pre-existing formatting difference in tests/integration/sessions/test_sessions.py; the other 1,832 files pass. The same difference is present on the unchanged base.
  • Required typecheck script reports 63 baseline diagnostics (62 errors and 1 warning), with identical output on the unchanged base after normalizing worktree paths. These include Windows-unavailable POSIX APIs, unavailable uvloop, and existing argument/type diagnostics. The changed source and test pass their focused typecheck.

The source was materialized from the matching 0.10.54 sdist and official fixed-SHA GitHub blobs; all tracked Python/TOML and test/script/publish files were verified before checking. Dependencies use the declared package pins plus locally installed test tools, not an exact uv.lock environment. Format/lint ran with uv run --no-project to use their script dependencies; typecheck ran with uv run --active --no-sync in the isolated environment, and the same script was rerun directly after the final strict UTF-8 boundary change with identical baseline diagnostics. The full unit suite, integration/e2e matrix, other platforms/interpreters and live provider calls were not run.

AI assistance: OpenAI Codex investigated, implemented and tested this change, with independent automated review. No personal human code review is claimed.

Required contributor question: “You're given a calfskin wallet for your birthday. How would you feel about using it?”
As the AI assistant preparing this contribution, I do not have personal feelings or use wallets, and I cannot represent the account owner's preference.

Decode GitHub repository path components once before filesystem installation and encode them for raw-content URLs. Preserve explicit filesystem paths and reference parsing, and reject path encodings that would be normalized into a different target.

Assisted-by: OpenAI Codex

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant